86/inconsistent message fix#157
Conversation
|
@Shwetraj1 is attempting to deploy a commit to the djed-solidity Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdds a Know Your Audience (KYA) acceptance modal with localStorage persistence and error handling, introduces a Footer component in the main layout, updates global and modal-related styling for responsive flex layout, and enhances reserve-ratio transaction validation with pre- and post-transaction checks and two new error codes. Changes
Sequence DiagramsequenceDiagram
participant User
participant App
participant Storage as localStorage
participant KYAModal
App->>Storage: read KYA acceptance flag on mount
alt storage available
Storage-->>App: return flag (true/false)
alt flag === false
App->>App: set modalVisible = true
App->>KYAModal: render(visible=true)
KYAModal->>User: show modal
User->>KYAModal: click "I understand and I agree"
KYAModal->>App: onAccept()
App->>Storage: write KYA acceptance flag
Storage-->>App: persist ack
App->>App: set modalVisible = false
KYAModal->>User: modal closes
else flag === true
App->>App: modal not shown
end
else storage unavailable
App->>App: set modalVisible = true (fallback)
App->>KYAModal: render(visible=true)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/components/organisms/Modals/KYAModal.module.scss (1)
44-44: Magic number in scrollable content height calculation.Line 44 uses
calc(80vh - 140px)where140pxappears to be a magic number representing the combined height of the title and footer sections. If padding or title height changes, this value would need manual adjustment.Consider using CSS variables for the fixed height components to make this calculation more maintainable:
+ --title-height: 60px; + --footer-height: 80px; + .scrollableContent { flex: 1; padding: 24px; overflow-y: auto; - max-height: calc(80vh - 140px); + max-height: calc(80vh - var(--title-height) - var(--footer-height));src/components/organisms/Modals/KYAModal.jsx (2)
44-48: Consider separating acceptance logic from modal closing.The
handleAcceptfunction calls bothonAccept()andonClose(). IfonAcceptthrows an error or fails (e.g., localStorage write fails), the modal will still close, potentially leaving the app in an inconsistent state. Consider letting the parent component handle closing after successful acceptance.const handleAccept = () => { - onAccept(); - onClose(); + onAccept(); // Let parent handle closing after successful acceptance };Then in
App.js, ensurehandleKYAAcceptcallssetShowKYAModal(false)after localStorage is set (which it already does on line 87).
6-42: Consider extracting KYA_CONTENT to a separate constants file.The large
KYA_CONTENTobject (37 lines) makes the component file longer and mixes content with presentation logic. Moving this to a dedicated constants file (e.g.,KYAContent.constants.js) would improve maintainability and make it easier to update content without touching component code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/App.js(3 hunks)src/App.scss(7 hunks)src/MainLayout.jsx(2 hunks)src/components/molecules/Footer/Footer.jsx(1 hunks)src/components/molecules/Footer/Footer.module.scss(1 hunks)src/components/organisms/Modals/KYAModal.jsx(1 hunks)src/components/organisms/Modals/KYAModal.module.scss(1 hunks)src/routes/reservecoin.jsx(2 hunks)src/utils/constants.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/routes/reservecoin.jsx (2)
src/context/AppProvider.jsx (2)
isRatioAboveMin(257-269)coinsDetails(54-54)src/utils/constants.js (2)
TRANSACTION_VALIDITY(6-21)TRANSACTION_VALIDITY(6-21)
src/MainLayout.jsx (1)
src/components/molecules/Footer/Footer.jsx (1)
Footer(32-57)
🔇 Additional comments (11)
src/App.scss (1)
26-38: LGTM! Layout changes properly support the new Footer.The transition from fixed
height: 100vhtomin-height: 100vhcombined with the flex column layout andflex: 1on the content area correctly implements a sticky footer pattern. This ensures the footer stays at the bottom even when content is sparse.src/MainLayout.jsx (1)
5-5: LGTM! Footer component properly integrated.The Footer import and placement after the Content section correctly implements the sticky footer pattern established by the layout changes in App.scss.
Also applies to: 91-91
src/App.js (3)
67-77: Good defensive localStorage handling.The try/catch block properly handles environments where localStorage may be unavailable (private browsing, disabled). The fallback to show the modal is a safe default that ensures users see important information.
90-94: Verify intended behavior: closing without accepting.The
handleKYAClosefunction allows users to close the modal without accepting, but doesn't persist this choice. This means the modal will reappear on the next visit. If this is intentional (requiring users to eventually accept), consider adding a comment explaining this behavior. If users should be able to permanently dismiss without accepting, this needs adjustment.Is this the intended UX flow? Should users be able to permanently dismiss the KYA modal without accepting?
101-105: Consider modal render timing relative to wallet connection.The KYAModal is rendered immediately on mount, potentially before users connect their wallet. Verify this is the intended flow - users might expect to connect their wallet first before seeing compliance modals.
Does the UX team intend for the KYA modal to appear before wallet connection, or should it be gated on wallet connection status?
src/components/molecules/Footer/Footer.module.scss (1)
1-69: LGTM! Well-structured and accessible footer styling.The styling properly implements a sticky footer with
margin-top: auto, includes responsive adjustments for mobile screens, and provides good accessibility support with focus-visible styles for keyboard navigation.src/components/molecules/Footer/Footer.jsx (1)
32-57: LGTM! Well-structured and accessible Footer component.The component properly implements semantic HTML with a
<footer>element, includes appropriate ARIA labels for accessibility, and correctly applies security attributes (rel="noopener noreferrer") to external links.src/components/organisms/Modals/KYAModal.jsx (1)
51-59: LGTM! Modal configuration is well-structured.The Modal component is properly configured with appropriate props, security attributes, and accessibility features.
src/utils/constants.js (1)
16-18: The three reserve ratio constants serve distinct purposes and are not redundant.
RESERVE_RATIO_ALREADY_LOW(line 16) is used when the current reserve ratio is already below the minimum (in RC selling, line 207 of reservecoin.jsx)RESERVE_RATIO_WOULD_DROP_LOW(line 17) is used when the reserve ratio is currently adequate but the transaction would cause it to drop below minimum (in RC selling, line 209 of reservecoin.jsx)RESERVE_RATIO_LOW(line 18) is used in a different context when buying SC and the transaction would drop the ratio below minimum (in stablecoin.jsx, line 130)Each constant is actively used in its appropriate context. No redundancy exists.
src/routes/reservecoin.jsx (2)
170-185: LGTM! Clear separation of current vs. future ratio checks.The implementation correctly distinguishes between the two scenarios:
- Current ratio check (lines 172-176) uses
coinsDetails?.unscaledPriceScfor the present state- Future ratio check (lines 179-185) uses
futureSCPriceand adjustsreserveBcby subtracting the transaction impactThis aligns well with the PR objective to provide context-aware error messages.
206-209: Validation order is correct.The check for
isCurrentRatioAboveMinimumappropriately precedes theisRatioAboveMinimumcheck. This ensures users see the systemic restriction message (RESERVE_RATIO_ALREADY_LOW) when the current ratio is already below minimum, rather than the transaction-specific message.One minor observation: both conditions will be false simultaneously when the current ratio is below minimum (since if current is below, future will also be below). The ordering correctly prioritizes the more informative "already low" message.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/molecules/Footer/Footer.jsx (2)
4-30: Previous placeholder link issue has been addressed.The placeholder social media links are now properly commented out, which resolves the concern from the previous review. The links won't render until real URLs are available.
However, consider removing the commented placeholder code entirely rather than keeping it in the source. If these links will be added later, a TODO comment without the example code would be cleaner and reduce maintenance burden.
If you prefer to remove the commented code:
{ label: 'GitHub', href: 'https://github.com/DjedAlliance/Djed-Solidity', }, - // Placeholder links - uncomment and add real URLs when available - // { - // label: 'Discord', - // href: 'https://discord.gg/your-invite-link', - // }, - // { - // label: 'X', - // href: 'https://x.com/your-profile', - // }, - // { - // label: 'LinkedIn', - // href: 'https://linkedin.com/company/your-page', - // }, + // TODO: Add Discord, X, and LinkedIn links when URLs are available ];
36-59: Well-implemented accessibility and security.The footer rendering includes proper security attributes (
rel="noopener noreferrer") on external links and good accessibility features witharia-labelattributes.Consider making the copyright year dynamic to avoid annual manual updates:
<div className={styles.copyright}> - © 2025 The Stable Order + © {new Date().getFullYear()} The Stable Order </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/molecules/Footer/Footer.jsx(1 hunks)
🔇 Additional comments (2)
src/components/molecules/Footer/Footer.jsx (2)
1-2: LGTM!Standard React imports are correct and necessary for the component.
32-34: LGTM!The defensive filtering logic is well-implemented and prevents invalid or placeholder links from rendering. The check for
startsWith('http')appropriately accepts both HTTP and HTTPS protocols.
|
@Zahnentferner All issues are solved just some nitpicks i didn't touch coz of commented codes can You please review My PR |
1 participant
Fix: Inconsistent Reserve Coin Sell Message
Problem #86
When users attempted to sell Reserve Coin, they received a generic error message "Reserve ratio would drop below the minimum" regardless of the actual scenario. This created confusion because the message didn't distinguish between:
The reserve ratio already being below the minimum requirement
The transaction would cause the reserve ratio to drop below the minimum
Solution
Enhanced the validation logic to provide distinct, context-aware error messages:
Current State Check: Added validation to check if the reserve ratio is already below the minimum before processing the transaction
Future State Check: Existing logic now only triggers if current state is acceptable, checking if the transaction would cause the ratio to drop
Changes Made
1.
src/utils/constants.js
Added two new transaction validity constants:
RESERVE_RATIO_ALREADY_LOW: Displayed when the system is currently in a restricted state
RESERVE_RATIO_WOULD_DROP_LOW: Displayed when the transaction would cause the restriction
2.
src/routes/reservecoin.jsx
Enhanced
updateSellTradeData()
function:
Added current reserve ratio validation using current blockchain state
Updated validation order to check current ratio before future ratio
Assigned appropriate error messages based on the specific scenario
User Impact
✅ Clear Communication: Users now understand exactly why their transaction is blocked
✅ Better UX: Distinct messages for different scenarios reduce confusion
✅ Transparency: Users can distinguish between systemic restrictions vs transaction-specific issues
Testing
Code review completed
Validation logic verified
Manual testing in development environment recommended
Files Changed
src/utils/constants.js
(+2 new constants)
src/routes/reservecoin.jsx
(~15 lines modified in validation logic)
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.